Reviewing files that changed from the base of the PR and between d3fa315 and 6fe79c0.

• ci/dash/lint-tidy.sh (1 hunks)
• src/.clang-tidy (1 hunks)
• src/bench/wallet_loading.cpp (0 hunks)
• src/bitcoin-cli.cpp (1 hunks)
• src/bitcoin-tx.cpp (1 hunks)
• src/blockfilter.cpp (1 hunks)
• src/coins.cpp (1 hunks)
• src/core_read.cpp (1 hunks)
• src/init.cpp (1 hunks)
• src/logging.cpp (1 hunks)
• src/netbase.cpp (3 hunks)
• src/node/blockstorage.cpp (1 hunks)
• src/qt/splashscreen.cpp (1 hunks)
• src/rpc/blockchain.cpp (1 hunks)
• src/rpc/mining.cpp (1 hunks)
• src/rpc/output_script.cpp (0 hunks)
• src/rpc/rawtransaction_util.cpp (1 hunks)
• src/rpc/util.cpp (1 hunks)
• src/script/descriptor.cpp (2 hunks)
• src/test/base58_tests.cpp (2 hunks)
• src/test/blockfilter_tests.cpp (1 hunks)
• src/test/fuzz/txorphan.cpp (1 hunks)
• src/test/key_io_tests.cpp (3 hunks)
• src/test/miniscript_tests.cpp (1 hunks)
• src/test/script_tests.cpp (1 hunks)
• src/test/sighash_tests.cpp (1 hunks)
• src/test/transaction_tests.cpp (4 hunks)
• src/test/util_tests.cpp (1 hunks)
• src/test/validation_block_tests.cpp (1 hunks)
• src/univalue/include/univalue.h (1 hunks)
• src/univalue/lib/univalue.cpp (2 hunks)
• src/util/message.cpp (0 hunks)
• src/util/strencodings.cpp (0 hunks)
• src/util/string.cpp (1 hunks)
• src/util/string.h (0 hunks)
• src/util/threadinterrupt.h (1 hunks)
• src/wallet/bdb.cpp (1 hunks)
• src/wallet/receive.cpp (1 hunks)
• src/wallet/rpc/backup.cpp (8 hunks)
• src/wallet/scriptpubkeyman.cpp (1 hunks)
• src/wallet/spend.cpp (1 hunks)
• src/wallet/test/wallet_tests.cpp (3 hunks)
• src/wallet/wallet.cpp (2 hunks)
• src/wallet/walletdb.cpp (2 hunks)
• test/functional/rpc_wipewallettxes.py (1 hunks)
• test/functional/wallet_balance.py (1 hunks)
• test/functional/wallet_import_rescan.py (5 hunks)
• test/functional/wallet_importdescriptors.py (2 hunks)
• test/lint/run-lint-format-strings.py (0 hunks)

• src/util/strencodings.cpp
• src/bench/wallet_loading.cpp
• test/lint/run-lint-format-strings.py
• src/util/message.cpp
• src/util/string.h
• src/rpc/output_script.cpp

• GitHub Check: linux64_ubsan-build / Build source
• GitHub Check: linux64_sqlite-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: linux64_fuzz-build / Build source
• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: mac-build / Build source
• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: arm-linux-build / Build source
• GitHub Check: win64-build / Build source
• GitHub Check: Lint / Run linters

src/util/threadinterrupt.h (1)

9-9: LGTM!

The include addition is well-placed and necessary to support the thread-safety annotations already in use throughout the file (e.g., EXCLUSIVE_LOCKS_REQUIRED on lines 26 and 28). This aligns with the PR's static analysis enhancements objective.

src/wallet/scriptpubkeyman.cpp (1)

1929-1929: LGTM! Efficient iteration with const reference.

This change eliminates unnecessary copies during iteration by using const auto& instead of auto. The semantic behavior is unchanged since key_pair is only read within the loop body.

src/test/fuzz/txorphan.cpp (1)

71-71: LGTM! Typo correction improves readability.

The comment typo fix from "poped" to "popped" is correct and improves code quality.

src/coins.cpp (1)

359-359: LGTM! Performance optimization eliminates unnecessary copies.

The change from by-value to const-reference binding avoids copying callback function objects during iteration, which is a beneficial performance optimization with no functional impact.

src/test/util_tests.cpp (1)

1307-1321: LGTM: NOLINT suppressions are correctly applied.

The inline NOLINT(*-use-after-move) comments appropriately silence false-positive static analysis warnings. The test intentionally uses the addresses of moved-from objects (&t2, &t3) to verify that the Tracker::origin pointer correctly references the original object after move operations. Taking the address of a moved-from object is safe and necessary for this test's correctness validation.

src/qt/splashscreen.cpp (1)

58-58: LGTM! Valid copy-elimination optimization.

Binding titleAddText as a const reference instead of copying the QString avoids an unnecessary allocation. All uses (lines 112, 116, 123) are read-only, and the lifetime is correctly scoped to the constructor where all references occur.

src/node/blockstorage.cpp (1)

451-451: Need to verify if this matches upstream Bitcoin Core or if this is a backport divergence. Let me check for any upstream context:

Binding temporary to const reference creates dangling reference.

The return type of GetBlocksDirPath() is fs::path (by value), not const fs::path&. Line 451 binds this temporary to a const lvalue reference:

const fs::path& blocksdir = gArgs.GetBlocksDirPath();

This creates a dangling reference—the temporary is destroyed at the statement's end, leaving blocksdir as a dangling pointer used in the loop at line 452. All other callers in the codebase (init.cpp:1160, init.cpp:2321, validation.cpp:2841, qt/clientmodel.cpp:255) pass the result directly to functions, never binding to a reference.

Fix: Remove the reference binding. Use const fs::path blocksdir = gArgs.GetBlocksDirPath(); (copy) or pass directly to fs::directory_iterator without intermediate storage.

⛔ Skipped due to learnings

Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.

Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.

Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.

src/netbase.cpp (1)

393-393: LGTM! Correct optimization.

These changes eliminate unnecessary assignments to recvr while preserving correct error handling. The variable recvr is properly initialized at line 441 and within the switch statement (lines 465-476) before being checked at line 479, so there are no stale value issues. This optimization reduces unnecessary variable assignments without affecting the SOCKS5 negotiation flow.

Also applies to: 416-416, 482-482

src/script/descriptor.cpp (1)

550-550: LGTM - backport pattern matches upstream.

The removal of explicit std::move() calls in favor of direct concatenation aligns with the backport objectives. While this changes from move to copy semantics for these temporary strings, the changes are semantically correct and match the upstream Bitcoin Core modifications.

Also applies to: 574-574

src/rpc/blockchain.cpp (1)

2530-2533: LGTM! Valid move optimization.

The change from const auto& script to CScript& script enables moving the script into descriptors on line 2533, avoiding an unnecessary copy. The sequence is correct: the script is first copied into needles (line 2532), then moved into descriptors (line 2533), which is safe since the moved-from element is never accessed again.

src/wallet/receive.cpp (1)

431-431: LGTM: Valid const-reference optimization.

The loop iterator now uses a const reference to avoid copying std::set<CTxDestination> objects during iteration. This is a standard performance optimization with no behavioral changes.

src/logging.cpp (1)

465-465: LGTM: Efficient const-reference binding.

Binding the result of ThreadGetInternalName() to a const reference avoids an unnecessary string copy. The temporary's lifetime is extended to the scope of the reference, making this safe.

src/util/string.cpp (1)

13-13: LGTM: Correct removal of ineffective std::move.

Removing std::move(search) is correct since search is a const std::string& parameter. Moving from a const reference is ineffective, so the previous code was misleading. This change improves code clarity.

ci/dash/lint-tidy.sh (1)

47-47: LGTM: Administrative linting update.

Adding src/util/string.cpp to the IWYU check list is appropriate given the changes to that file in this PR.

src/wallet/spend.cpp (1)

316-316: LGTM: Optimization to enable move semantics.

Changing the loop variable to a non-const reference allows moving coin objects at line 320 (std::move(coin)) instead of copying them. Since AvailableCoinsListUnspent(wallet).all() returns a temporary vector, iterating with non-const references and moving the elements is a valid optimization.

src/wallet/bdb.cpp (1)

446-446: LGTM: Const-reference optimization for map iteration.

Using a const reference when iterating over m_databases avoids copying map entries. This is a standard performance optimization with no behavioral changes.

src/bitcoin-cli.cpp (1)

951-951: LGTM: Const-reference binding avoids string copy.

Binding wallet_name as a const reference to the result of wallet.get_str() avoids an unnecessary string copy. Both uses of wallet_name (lines 952 and 954) are within the same scope, making this optimization safe.

src/blockfilter.cpp (1)

173-173: LGTM: Const-reference optimization for map iteration.

Using a const reference when iterating over g_filter_types avoids copying map entries. This is a standard performance optimization consistent with the broader changes in this PR.

src/test/validation_block_tests.cpp (1)

185-191: Micro-copy elimination in drain loop looks good

Binding block as const auto& avoids extra shared_ptr copies; behavior unchanged.

src/rpc/mining.cpp (1)

755-762: Const-ref bind of longpoll id is fine

Removes a redundant string copy; scope-limited use is safe.

src/init.cpp (1)

1436-1444: OK to bind datadir as const reference

Removes one fs::path copy; usage stays within scope; no behavior change.

src/rpc/util.cpp (1)

100-108: ParseHashV copy-avoidance is fine

Const-ref to the returned string is scoped and safe; logic unchanged.

src/test/miniscript_tests.cpp (1)

119-121: UDL import for miniscript tests is correct

Bringing operator"" _mst into scope is needed for the tests; good addition.

src/core_read.cpp (1)

184-199: Sighash parsing: const-ref bind LGTM

Minor copy elimination with no semantic change.

src/rpc/rawtransaction_util.cpp (1)

149-150: Avoid UniValue copies in ParsePrevouts

Binding prevTxs and prevOut as const refs reduces allocations; behavior unchanged.

Also applies to: 156-157

src/wallet/test/wallet_tests.cpp (3)

784-786: LGTM: rescan path counts two AddToWallet events.

Expectation of 2 while queue is blocked looks correct and stable.

---

797-799: LGTM: stale notifications counted after unblocking.

Expecting the second pair of events (total 4) after SyncWithValidationInterfaceQueue() is correct.

---

821-821: LGTM: immediate notification path counts once.

Asserting 2 in the second scenario (no double-count) matches the intended timing.

src/.clang-tidy (1)

4-14: Tighter static analysis config looks good.

Enabling performance/bugprone checks and gating as errors is fine; expect some churn in first run.
Please confirm lint CI passes after this change on all supported compilers.

Also applies to: 15-26, 27-29

src/test/base58_tests.cpp (1)

27-27: Bind JSON test elements by const reference.

Nice micro-optimization; keeps semantics intact.

Also applies to: 49-49

src/test/script_tests.cpp (1)

884-884: Const-ref bind for test element — good.

Avoids UniValue copies; no logic changes.

src/bitcoin-tx.cpp (1)

558-606: Const-ref prevOut in sign loop — correct and efficient.

No behavior change; keeps validations identical while avoiding copies.

src/test/blockfilter_tests.cpp (1)

147-147: Const-ref bind for test entry.

Removes needless UniValue copies; fine.

src/test/sighash_tests.cpp (1)

165-165: Const-ref bind for sighash JSON test entry.

Good small cleanup; keeps behavior identical.

src/test/key_io_tests.cpp (1)

31-31: LGTM! Efficient const-reference bindings eliminate unnecessary copies.

The conversion from by-value to const-reference for test loop variables is a clean performance optimization that avoids copying UniValue objects while preserving read-only access semantics.

Also applies to: 89-89, 129-129

src/test/transaction_tests.cpp (2)

156-156: LGTM! Efficient const-reference bindings for test variables.

The conversion avoids unnecessary UniValue copies while maintaining read-only access within the test loops.

Also applies to: 240-240

---

175-175: LGTM! Safe const-reference binding to get_array() result.

Binding a const reference to the result of get_array() is safe here—either it aliases an existing reference (if get_array() returns by reference) or extends the lifetime of a temporary (if it returns by value). Usage is read-only within the loop scope.

Also applies to: 259-259

src/univalue/lib/univalue.cpp (4)

104-109: LGTM! Move-semantics optimization for push_back.

The signature change to pass UniValue by value enables move semantics: callers passing rvalues incur no copy, while lvalues are copied into the parameter, then the parameter is moved into storage. This is more efficient than the previous const-reference approach which always copied.

---

118-124: LGTM! Move-semantics optimization for __pushKV.

Both key and value parameters are now passed by value and moved into internal storage, enabling efficient handling of rvalue arguments while maintaining correctness for lvalues.

---

126-135: LGTM! Move-semantics optimization for pushKV with correct conditional logic.

The method correctly handles key replacement (line 132) and new key insertion (line 134) using move semantics. When replacing an existing key's value, the new value is moved into place; when inserting a new key-value pair, both are moved to __pushKV.

---

137-144: LGTM! Move-semantics optimization for pushKVs.

The parameter is passed by value and its keys/values are moved into the target object within the loop. This efficiently transfers ownership from the source object.

src/univalue/include/univalue.h (1)

80-80: ✓ Public API move semantics implementation verified and correct.

The method signatures at lines 80 and 85–87 have been properly updated from const-reference to by-value parameters. The implementation (univalue.cpp line 104) correctly uses std::move(val) to forward the parameter, enabling move semantics optimization for rvalues while maintaining backward compatibility for lvalues through implicit copy.

The extensive call sites throughout the codebase (700+ occurrences) all work correctly with the new signatures. This is a standard C++11 optimization pattern with no breaking changes.

test/functional/rpc_wipewallettxes.py (1)

36-37: Test change correctly implements mempool-aware rescan behavior from Bitcoin Core bitcoin#25351 backport.

The assertion change from 103 to 104 reflects the upstream Bitcoin Core bitcoin#25351 backport which implements "Rescan mempool for transactions as well." After wipewallettxes(True) removes the unconfirmed transaction from the wallet, rescanblockchain() now recovers it from the mempool, correctly resulting in 104 transactions instead of 103. This change matches the exact modifications in the upstream backport commit.

test/functional/wallet_balance.py (1)

283-305: LGTM: Mempool-aware import test correctly validates new behavior.

The test appropriately verifies that mempool transactions are recognized during import operations:

1. After importaddress, the mempool transaction appears in watchonly.untrusted_pending
2. After importprivkey, the transaction moves to mine.untrusted_pending

The conditional check at lines 298-299 with the explanatory comment is acceptable for this backport.

src/wallet/wallet.cpp (2)

1688-1689: LGTM: Documentation accurately describes mempool scanning behavior.

The updated documentation clearly states that mempool will be scanned when max_height is not set, which aligns with the implementation.

---

1810-1813: LGTM: Mempool scanning implementation is correct and thread-safe.

The implementation correctly:

1. Scans mempool only when max_height is not set (full rescan scenario)
2. Uses WITH_LOCK(cs_wallet, ...) for thread safety, consistent with patterns seen elsewhere in the codebase
3. Executes after block scanning completes, ensuring proper ordering

test/functional/wallet_importdescriptors.py (2)

430-431: LGTM: Transaction ID now captured for mempool verification.

Storing send_txid enables the subsequent assertions at lines 458-459 to verify mempool transaction visibility after descriptor import.

---

457-459: Rewritten Review Comment: Remove incorrect explanation of address derivation

The assertions at lines 457-459 correctly verify that mempool transactions are recognized after importing descriptors to a watch-only wallet:

1. The transaction from wmulti_priv exists in the mempool
2. The transaction is correctly included in wmulti_pub.listunspent(0) (unconfirmed outputs)

However, the change address at line 457 ('8y2sLiPQnB81bAeiRvwbjozJXnCCNH2nHb') is different from line 426 ('91WxMwg2NHD1PwHChhbAkeCN6nQ8ikdLEx') because wmulti_priv and wmulti_pub are separate wallets with different key material (private vs. public keys). The address sequencing reflects normal index advancement (0 consumed, 1 pending, 2 available), not a mempool-triggered update. Line 456's comment should clarify this derives from the internal (change) descriptor path 84h/1h/0h/*, and the keypool assertion at line 460 confirms correct index consumption.

src/wallet/rpc/backup.cpp (6)

94-101: Consistent rescan guidance for importprivkey

The added explanation about when rescan can be safely set to false, plus the updated argument description (“Scan the chain and mempool for wallet transactions.”), is clear and aligns with mempool-aware rescans. No issues.

---

213-214: importaddress help text matches new rescan semantics

The extra note about using rescanwallet when rescan=false and the mempool-inclusive wording for the rescan argument look consistent with importprivkey and the broader behavior change.

Also applies to: 223-223

---

411-412: importpubkey rescan docs kept in sync

Same pattern here: guidance for rescan=false and the “chain and mempool” description are consistent with the other RPCs. Good to keep these three imports aligned.

Also applies to: 417-417

---

493-493: importwallet: explicit blockchain + mempool rescan note

The new sentence about rescanning both blockchain and mempool and pointing users to getwalletinfo for progress is helpful and matches the intended behavior of the post-import rescan.

---

1484-1485: importmulti: clearer rescan option text

The additional explanation around rescan=false and the updated options.rescan description (“Scan the chain and mempool… after all imports.”) are accurate and consistent with the other RPCs.

Also applies to: 1529-1529

---

1841-1841: importdescriptors timestamp help mentions mempool

The timestamp help now explicitly says that blocks up to 2 hours before the earliest timestamp and the mempool are scanned, which matches the mempool-rescan behavior under test.

test/functional/wallet_import_rescan.py (4)

72-104: Variant.check: dynamic confirmations and “trusted” field assertions

The updated check method correctly derives confirmations from the node’s current height and an optional confirmation_height, while using 0 for mempool transactions when no height is passed. Matching the address entry’s confirmations to the tx, and asserting that confirmed txs lack a "trusted" field, is consistent with the new wallet RPC semantics and supports the mempool-rescan tests.

---

162-165: setup_network: add whitelist for second startup

Restarting all nodes with [email protected] ensures mempool synchronization and P2P behavior are not limited by bans/DoS scoring during the import/rescan checks. This is a targeted change and still respects the per-node prune/txindex configuration established in add_nodes.

---

205-210: Loop simplification when sending second-round transactions

Switching to for variant in IMPORT_VARIANTS: (dropping the unused index) preserves behavior while slightly simplifying the code. No functional impact.

---

221-258: New mempool-rescan coverage path looks correct

The added mempool-specific test section:

• Filters out late-timestamp and pruned variants, focusing on unpruned nodes where mempool rescans are meaningful.
• Keeps the block timestamp fixed so only mempool contents change.
• Creates one unconfirmed transaction per variant, syncs mempools, and then reuses the existing Variant import logic and check() to assert:
  • rescan=true imports see the mempool transaction with confirmations==0 and the expected balance and tx count.
  • rescan=false imports see no txs and zero balance.

Node selection via ImportNode(variant.prune, expect_rescan) matches the earlier parts of the test, and labels are distinct (“mempool label …”), so per-label assertions remain isolated even though nodes are reused. This gives good coverage of mempool-inclusive rescan behavior without altering non-mempool cases.

Reviewing files that changed from the base of the PR and between 6fe79c0 and c3d5b2e.

• ci/dash/lint-tidy.sh (1 hunks)
• src/.clang-tidy (1 hunks)
• src/bench/wallet_loading.cpp (0 hunks)
• src/bitcoin-cli.cpp (1 hunks)
• src/bitcoin-tx.cpp (1 hunks)
• src/blockfilter.cpp (1 hunks)
• src/coins.cpp (1 hunks)
• src/core_read.cpp (1 hunks)
• src/init.cpp (1 hunks)
• src/logging.cpp (1 hunks)
• src/netbase.cpp (3 hunks)
• src/node/blockstorage.cpp (1 hunks)
• src/qt/splashscreen.cpp (1 hunks)
• src/rpc/blockchain.cpp (1 hunks)
• src/rpc/mining.cpp (1 hunks)
• src/rpc/output_script.cpp (0 hunks)
• src/rpc/rawtransaction_util.cpp (1 hunks)
• src/rpc/util.cpp (1 hunks)
• src/script/descriptor.cpp (2 hunks)
• src/test/base58_tests.cpp (2 hunks)
• src/test/blockfilter_tests.cpp (1 hunks)
• src/test/fuzz/txorphan.cpp (1 hunks)
• src/test/key_io_tests.cpp (3 hunks)
• src/test/miniscript_tests.cpp (1 hunks)
• src/test/script_tests.cpp (1 hunks)
• src/test/sighash_tests.cpp (1 hunks)
• src/test/transaction_tests.cpp (4 hunks)
• src/test/util_tests.cpp (1 hunks)
• src/test/validation_block_tests.cpp (1 hunks)
• src/univalue/include/univalue.h (1 hunks)
• src/univalue/lib/univalue.cpp (2 hunks)
• src/util/message.cpp (0 hunks)
• src/util/strencodings.cpp (0 hunks)
• src/util/string.cpp (1 hunks)
• src/util/string.h (0 hunks)
• src/util/threadinterrupt.h (1 hunks)
• src/wallet/bdb.cpp (1 hunks)
• src/wallet/receive.cpp (1 hunks)
• src/wallet/rpc/backup.cpp (8 hunks)
• src/wallet/scriptpubkeyman.cpp (1 hunks)
• src/wallet/spend.cpp (1 hunks)
• src/wallet/test/wallet_tests.cpp (3 hunks)
• src/wallet/wallet.cpp (2 hunks)
• src/wallet/walletdb.cpp (2 hunks)
• test/functional/rpc_wipewallettxes.py (1 hunks)
• test/functional/wallet_balance.py (1 hunks)
• test/functional/wallet_import_rescan.py (5 hunks)
• test/functional/wallet_importdescriptors.py (2 hunks)

• src/util/strencodings.cpp
• src/bench/wallet_loading.cpp
• src/util/message.cpp
• src/util/string.h
• src/rpc/output_script.cpp

• src/wallet/wallet.cpp

• src/test/blockfilter_tests.cpp
• src/util/string.cpp
• src/test/base58_tests.cpp
• src/test/sighash_tests.cpp
• src/test/transaction_tests.cpp
• src/coins.cpp
• src/core_read.cpp
• src/wallet/scriptpubkeyman.cpp
• src/node/blockstorage.cpp
• ci/dash/lint-tidy.sh
• src/wallet/receive.cpp
• src/.clang-tidy
• src/test/util_tests.cpp
• src/univalue/include/univalue.h
• src/init.cpp
• src/test/validation_block_tests.cpp
• src/wallet/spend.cpp
• src/bitcoin-tx.cpp
• src/test/fuzz/txorphan.cpp
• src/rpc/util.cpp
• src/logging.cpp

• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: mac-build / Build source
• GitHub Check: arm-linux-build / Build source
• GitHub Check: linux64_sqlite-build / Build source
• GitHub Check: linux64_ubsan-build / Build source
• GitHub Check: linux64_fuzz-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: win64-build / Build source
• GitHub Check: Lint / Run linters

src/util/threadinterrupt.h (1)

9-9: LGTM! Proper include for thread safety annotations.

Adding #include <threadsafety.h> is correct since the file uses EXCLUSIVE_LOCKS_REQUIRED annotations on lines 26 and 28. This follows the "include what you use" principle and makes the header self-contained.

src/test/script_tests.cpp (1)

884-884: LGTM! Avoids unnecessary copy.

The change from by-value to const-reference binding eliminates an unnecessary copy of the UniValue object while preserving correctness. Since test is only read (never modified), binding to a const reference is safe and improves efficiency.

src/test/key_io_tests.cpp (1)

31-31: LGTM! Consistent copy elimination across all test cases.

All three changes consistently eliminate unnecessary copies by binding to const references. Each test variable is only read (never modified), making the const-reference binding both safe and more efficient.

Also applies to: 89-89, 129-129

src/test/miniscript_tests.cpp (1)

119-121: LGTM! Proper handling of clang-tidy false positive.

The addition of the using declaration for the _mst literal operator (used on line 145) is correct, and the NOLINTNEXTLINE directive appropriately suppresses a known clang-tidy false positive documented in the referenced LLVM issue.

test/functional/rpc_wipewallettxes.py (1)

36-37: LGTM! Test correctly verifies mempool-inclusive rescan behavior.

The updated assertion correctly reflects the backported behavior change where rescanblockchain() (when called without max_height) now includes mempool transactions. After wiping the unconfirmed transaction at line 33, the rescan properly recovers it from the mempool, bringing the total from 103 to 104 transactions.

src/wallet/rpc/backup.cpp (3)

94-95: LGTM: Consistent help text improvements across import RPCs.

The help text updates consistently enhance user guidance by:

• Clarifying that rescanning includes both chain and mempool
• Explaining when rescan=false is appropriate (key never used)
• Noting that rescanwallet should be called if the key was used but rescan=false was set

These documentation improvements are consistent across importprivkey, importaddress, and importpubkey RPCs.

Also applies to: 100-100, 213-214, 223-223, 411-412, 417-417

---

493-493: LGTM: Clear documentation of rescan behavior.

The updated help text clearly indicates that both the blockchain and mempool will be rescanned after a successful import, improving user understanding of the operation's scope.

---

1484-1485: LGTM: Completed documentation updates for descriptor imports.

The help text updates for importmulti and importdescriptors complete the consistent documentation improvements across all import-related RPCs. The changes appropriately indicate that rescanning includes mempool transactions in addition to blockchain transactions.

Also applies to: 1529-1529, 1841-1841

src/script/descriptor.cpp (2)

550-550: LGTM!

The concatenation logic is correct. The change from move semantics to direct concatenation is intentional from the upstream backport and doesn't introduce any functional issues.

---

574-574: LGTM!

The concatenation logic is correct and consistent with the change at line 550. The modification is part of the upstream backport and maintains correct functionality.

src/qt/splashscreen.cpp (1)

58-58: LGTM – const reference binding is safe here.

The change from by-value to const reference avoids a copy. Since all uses of titleAddText (lines 112, 116, 123) occur within the constructor scope, temporary lifetime extension ensures the binding remains valid throughout its usage.

src/netbase.cpp (1)

393-393: LGTM! Clean refactoring of error handling.

The changes eliminate unnecessary assignments to recvr when the return value is only needed for an immediate comparison. The variable is still properly used for the more complex error handling in the middle section (lines 441-479) where the specific error type matters. This simplifies the code without changing behavior.

Also applies to: 416-416, 482-482

src/rpc/blockchain.cpp (1)

2530-2533: LGTM! Performance optimization through move semantics.

The change from const auto& script to CScript& script enables the move operation on line 2533, reducing overhead. The implementation is correct:

1. Line 2531: Script is used (read-only) to infer the descriptor
2. Line 2532: Script is copied into the needles set
3. Line 2533: Script is moved into descriptors map as the key

This optimizes from two copies to one copy + one move, while maintaining correctness since the script isn't accessed after the move.

test/functional/wallet_importdescriptors.py (1)

430-431: LGTM! Captures transaction ID for subsequent validation.

The change from discarding the return value to capturing send_txid enables the assertions below to verify mempool and unspent output state, which aligns with the mempool-rescan feature being tested.

test/functional/wallet_import_rescan.py (6)

79-79: LGTM! Minor formatting adjustment.

Blank line removed for consistency.

---

91-98: LGTM! Correctly handles mempool transactions with dynamic confirmations.

The updated logic computes confirmations dynamically based on whether confirmation_height is provided. When confirmation_height is None (mempool transactions), confirmations correctly evaluates to 0. The assertion on line 97 ensures that confirmed transactions don't have a "trusted" field, which is the expected behavior.

---

102-102: LGTM! Reuses the computed confirmations value.

This ensures consistency between transaction and address confirmation checks.

---

161-161: LGTM! Whitelist flag accelerates test execution.

The whitelist flag speeds up transaction relay and mempool synchronization, which is beneficial for the mempool-rescan tests added below.

---

166-166: LGTM! Minor formatting adjustment.

Blank line removed for consistency.

---

220-257: LGTM! Comprehensive mempool rescan test coverage.

The new test section properly validates mempool-inclusive rescan behavior:

• Filters out late_timestamp and pruned variants that aren't applicable to mempool testing
• Creates mempool transactions with unique amounts for each import variant
• Verifies mempool size matches expectations and synchronizes across nodes
• For each variant, validates that:
  • Imports with rescan=yes correctly pick up mempool transactions
  • Imports with rescan=no correctly ignore mempool transactions

The test design aligns with the PR objective of validating wallet rescanning that includes mempool when no max_height is specified.

test/functional/wallet_balance.py (1)

283-305: LGTM! Validates mempool-inclusive import behavior for legacy wallets.

The new test block correctly validates that:

1. importaddress flow: After importing an address to a fresh wallet, the mempool transaction appears in watchonly.untrusted_pending (line 293)
2. importprivkey flow: After importing the private key, the balance moves from watchonly to mine.untrusted_pending (lines 294-299)
3. Fresh wallet importprivkey flow: A new wallet with importprivkey directly sees the balance in mine.untrusted_pending (lines 302-304)

The conditional check on line 298 handles Dash-specific behavior where watchonly is omitted when the balance is zero, as noted in the TODO comment on line 297. This divergence from Bitcoin Core is documented and intentional.

src/wallet/bdb.cpp (1)

438-448: Const-ref iteration over m_databases is correct and avoids copies

Using for (const auto& it : m_databases) here is safe and slightly more efficient since you only read it.first to build filenames, without changing any behavior.

src/blockfilter.cpp (1)

171-176: Efficient iteration in AllBlockFilterTypes

Switching to for (const auto& entry : g_filter_types) is a straightforward copy-elision that leaves the logic and thread-safety of the std::call_once initialization unchanged.

src/rpc/mining.cpp (1)

755-762: Longpoll lpstr const-ref binding is safe

lpstr is a const reference to the temporary returned by lpval.get_str(), and it’s only used within the same block, so lifetime extension keeps it valid while needed; behavior matches the previous by-value code while avoiding an extra copy.

src/bitcoin-cli.cpp (1)

949-955: Const-ref wallet_name binding is lifetime-safe and preserves behavior

Binding wallet_name as const std::string& to wallet.get_str() is safe here (the temporary’s lifetime is extended to the scope of the reference) and it’s only copied into the optional rpcwallet parameter, so this just avoids an intermediate string copy with no semantic change.

src/rpc/rawtransaction_util.cpp (1)

148-178: Const-ref bindings in ParsePrevouts are safe and reduce copies

Using const UniValue& prevTxs = prevTxsUnival.get_array(); and const UniValue& prevOut = p.get_obj(); is safe here—each temporary’s lifetime is extended to the reference scope, and the values are only read within that scope—so behavior is unchanged while avoiding a couple of UniValue copies.

src/wallet/test/wallet_tests.cpp (3)

784-785: LGTM! Helpful clarifying comment.

The comment clearly explains that the two AddToWallet events correspond to block_tx and mempool_tx detected during the wallet rescan, despite notifications being blocked.

---

797-799: LGTM! Clear explanation of event replay.

The comment correctly describes that the AddToWallet count increases to 4 because the queued notifications are now processed, causing the same transactions to trigger AddToWallet events a second time.

---

821-821: Correct expectation adjustment for synchronous notification scenario.

The change from 4 to 2 is correct here because in this test path, transactions are created and notifications are processed synchronously (via SyncWithValidationInterfaceQueue on Line 818) rather than being queued and replayed. This results in only 2 AddToWallet events instead of 4.

src/wallet/walletdb.cpp (3)

887-891: LGTM! Efficient iteration with const reference.

Using const auto& instead of auto avoids unnecessary copies of descriptor cache entries during iteration, improving performance without changing behavior.

---

894-902: LGTM! Optimized descriptor key iteration.

The change to const auto& eliminates unnecessary copies of descriptor key pairs, which improves wallet loading performance since these are non-trivial objects.

---

904-912: LGTM! Consistent optimization for encrypted keys.

Using const auto& here maintains consistency with the other iterator optimizations and avoids copying crypted key data during wallet loading.

src/univalue/lib/univalue.cpp (4)

104-109: LGTM! Modern C++ move semantics optimization.

The change from pass-by-const-reference to pass-by-value with std::move enables efficient handling of both lvalues (copy once) and rvalues (move directly), improving performance when pushing temporary UniValue objects.

---

118-124: LGTM! Consistent move optimization for key-value pairs.

Both parameters now use pass-by-value with std::move, allowing efficient handling of temporary keys and values. This is consistent with the push_back optimization and follows modern C++ best practices.

---

126-135: LGTM! Proper move semantics in both code paths.

The function correctly uses std::move when replacing an existing value (Line 132) and when delegating to __pushKV for new keys (Line 134), ensuring efficient handling in both scenarios.

---

137-144: LGTM! Efficient object merging with move semantics.

Pass-by-value with std::move in the loop enables efficient transfer of key-value pairs when merging objects, particularly beneficial when the source object is a temporary.

Reviewing files that changed from the base of the PR and between ab0119c and 3c4ff33.

• ci/dash/lint-tidy.sh (1 hunks)
• src/.clang-tidy (1 hunks)
• src/bench/load_external.cpp (1 hunks)
• src/bench/wallet_loading.cpp (0 hunks)
• src/bitcoin-cli.cpp (1 hunks)
• src/bitcoin-tx.cpp (1 hunks)
• src/blockfilter.cpp (1 hunks)
• src/bls/bls_worker.cpp (1 hunks)
• src/coins.cpp (1 hunks)
• src/core_read.cpp (1 hunks)
• src/evo/creditpool.cpp (1 hunks)
• src/evo/mnauth.cpp (1 hunks)
• src/evo/specialtxman.cpp (1 hunks)
• src/init.cpp (1 hunks)
• src/instantsend/instantsend.cpp (1 hunks)
• src/llmq/blockprocessor.cpp (1 hunks)
• src/llmq/snapshot.cpp (1 hunks)
• src/logging.cpp (1 hunks)
• src/masternode/meta.cpp (1 hunks)
• src/netbase.cpp (3 hunks)
• src/node/blockstorage.cpp (1 hunks)
• src/qt/governancelist.cpp (1 hunks)
• src/qt/splashscreen.cpp (2 hunks)
• src/qt/transactionrecord.cpp (0 hunks)
• src/rpc/blockchain.cpp (1 hunks)
• src/rpc/coinjoin.cpp (1 hunks)
• src/rpc/evo.cpp (0 hunks)
• src/rpc/masternode.cpp (0 hunks)
• src/rpc/mining.cpp (3 hunks)
• src/rpc/output_script.cpp (0 hunks)
• src/rpc/rawtransaction_util.cpp (1 hunks)
• src/rpc/util.cpp (3 hunks)
• src/script/descriptor.cpp (2 hunks)
• src/test/base58_tests.cpp (2 hunks)
• src/test/blockfilter_tests.cpp (1 hunks)
• src/test/coinstatsindex_tests.cpp (0 hunks)
• src/test/evo_deterministicmns_tests.cpp (1 hunks)
• src/test/evo_trivialvalidation.cpp (1 hunks)
• src/test/fuzz/txorphan.cpp (1 hunks)
• src/test/key_io_tests.cpp (3 hunks)
• src/test/miniscript_tests.cpp (1 hunks)
• src/test/script_tests.cpp (1 hunks)
• src/test/sighash_tests.cpp (1 hunks)
• src/test/transaction_tests.cpp (4 hunks)
• src/test/util_tests.cpp (1 hunks)
• src/test/validation_block_tests.cpp (1 hunks)
• src/univalue/include/univalue.h (1 hunks)
• src/univalue/lib/univalue.cpp (2 hunks)
• src/util/message.cpp (0 hunks)
• src/util/strencodings.cpp (0 hunks)
• src/util/string.cpp (1 hunks)
• src/util/string.h (0 hunks)
• src/util/threadinterrupt.h (1 hunks)
• src/wallet/bdb.cpp (1 hunks)
• src/wallet/receive.cpp (1 hunks)
• src/wallet/scriptpubkeyman.cpp (1 hunks)
• src/wallet/spend.cpp (1 hunks)
• src/wallet/test/bip39_tests.cpp (1 hunks)
• src/wallet/test/wallet_tests.cpp (3 hunks)
• src/wallet/walletdb.cpp (2 hunks)

• src/rpc/output_script.cpp
• src/rpc/masternode.cpp
• src/util/strencodings.cpp
• src/rpc/evo.cpp
• src/bench/wallet_loading.cpp
• src/test/coinstatsindex_tests.cpp
• src/util/message.cpp
• src/qt/transactionrecord.cpp
• src/util/string.h

• src/init.cpp
• src/test/util_tests.cpp
• src/node/blockstorage.cpp
• src/.clang-tidy
• src/wallet/bdb.cpp
• src/wallet/test/bip39_tests.cpp
• src/test/blockfilter_tests.cpp
• src/llmq/blockprocessor.cpp
• src/qt/governancelist.cpp
• src/blockfilter.cpp
• src/test/base58_tests.cpp
• src/test/script_tests.cpp
• src/core_read.cpp
• src/rpc/mining.cpp
• src/test/sighash_tests.cpp
• src/test/transaction_tests.cpp
• src/wallet/spend.cpp
• src/instantsend/instantsend.cpp
• src/wallet/walletdb.cpp
• src/test/evo_deterministicmns_tests.cpp
• src/masternode/meta.cpp
• src/bitcoin-tx.cpp
• src/rpc/blockchain.cpp
• src/test/key_io_tests.cpp
• src/evo/creditpool.cpp
• src/rpc/coinjoin.cpp

• GitHub Check: arm-linux-build / Build source
• GitHub Check: mac-build / Build source
• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: linux64_ubsan-build / Build source
• GitHub Check: linux64_fuzz-build / Build source
• GitHub Check: linux64_sqlite-build / Build source
• GitHub Check: win64-build / Build source
• GitHub Check: Lint / Run linters

src/test/validation_block_tests.cpp (1)

185-185: LGTM! Optimizes loop iteration to avoid unnecessary copies.

The change from auto to const auto& avoids copying shared_ptr objects in each iteration, which is beneficial in this hot loop executed across multiple threads. Since the loop body only reads from block and doesn't modify it, using a const reference is both correct and more efficient.

src/test/fuzz/txorphan.cpp (1)

71-71: LGTM! Clean hygiene improvements.

The spelling correction ("popped" instead of "poped") and the removal of the unused #include <algorithm> header improve code quality without affecting functionality. The code uses std::swap from <utility> (line 25), so the <algorithm> header is indeed unnecessary.

src/util/threadinterrupt.h (1)

9-9: LGTM!

The explicit include of threadsafety.h is appropriate since the class uses thread safety annotations (EXCLUSIVE_LOCKS_REQUIRED) on lines 26 and 28. This makes the dependency explicit rather than relying on transitive includes.

src/bls/bls_worker.cpp (1)

18-18: LGTM! Efficient const-reference binding.

The change from auto item to const auto& item avoids unnecessary copies while preserving the validation and duplicate-detection logic.

src/coins.cpp (1)

359-359: LGTM! Good optimization.

Using const auto& instead of auto avoids unnecessary copies of the callback objects during iteration, which is especially beneficial if the callbacks are std::function or similar types that can be expensive to copy.

src/test/miniscript_tests.cpp (1)

119-121: LGTM! User-defined literal operator properly declared.

The using declaration brings the _mst operator into scope for cleaner syntax (used on line 145). The NOLINTNEXTLINE directive appropriately suppresses a false positive from clang-tidy regarding unused using declarations, with a reference to the relevant LLVM issue.

src/netbase.cpp (1)

393-393: LGTM! Clean refactoring of error checking.

The changes eliminate unnecessary intermediate variable assignments by comparing InterruptibleRecv results directly to IntrRecvError::OK when the specific error type doesn't need to be discriminated. The logic remains unchanged, and cases that do need to distinguish error types (like checking for Timeout at line 442) still properly store the result in recvr.

Also applies to: 416-416, 482-482

src/script/descriptor.cpp (2)

543-553: LGTM - Backport change is correct.

The removal of std::move(tmp) at line 550 is consistent with the upstream Bitcoin Core backport. While this changes the operation from move to copy, the functional behavior remains correct and the performance impact is negligible.

---

555-581: LGTM - Backport change is correct.

The removal of std::move(tmp) at line 574 is consistent with the upstream Bitcoin Core backport. Note that line 579 retains move semantics for the final concatenation, indicating this was a deliberate upstream change rather than a blanket removal of all moves. The code remains functionally correct.

src/test/evo_trivialvalidation.cpp (1)

51-51: LGTM: Loop variable optimization reduces unnecessary copies.

Changing the loop variable from by-value to const-reference avoids copying UniValue objects on each iteration while preserving the existing read-only access pattern.

src/wallet/scriptpubkeyman.cpp (1)

1929-1929: LGTM: Loop iteration optimization avoids unnecessary pair copies.

Changing from by-value to const-reference iteration avoids copying each key-value pair from m_map_crypted_keys. The loop body only reads from key_pair, making const-reference the appropriate choice.

src/wallet/test/wallet_tests.cpp (2)

784-799: LGTM: Test expectations updated to reflect wallet rescan behavior changes.

The updated assertions and clarifying comments correctly reflect the wallet's changed behavior regarding mempool transaction handling during rescans. The test now expects:

• 2 AddToWallet events after initial rescan (block_tx and mempool_tx detected)
• 4 total events after processing queued notifications (stale events counted again)

The comments help document the event counting logic at each checkpoint.

---

821-821: LGTM: Assertion matches immediate notification handling scenario.

The expectation of 2 AddToWallet events is correct for the scenario where block and mempool transactions are created after wallet load and notifications are processed immediately.

src/qt/splashscreen.cpp (2)

119-119: LGTM!

Inlining the getBadgeColor() call is safe and reduces an intermediate variable. The temporary (if any) lives until the end of the full expression, which is after fillRect completes.

---

58-58: No issues found. The getTitleAddText() method returns const QString&, making the const reference binding on line 58 safe. The reference points to an internal member that persists for the program's lifetime, so all subsequent uses are valid.

src/llmq/snapshot.cpp (1)

212-218: LGTM: Refactoring to direct output parameter with defensive reset.

The change correctly refactors from using a local variable to writing directly into response.mnListDiffAtHMinus4C. The defensive reset on line 216 ensures the field is in a clean state if BuildSimplifiedMNListDiff fails after partial modification, preserving the previous behavior where assignment would only occur on success.

Note: This is the only BuildSimplifiedMNListDiff call in the file that explicitly resets its output parameter on failure, which appears intentional for the optional extraShare field.

src/univalue/lib/univalue.cpp (4)

104-109: LGTM! Correct move-semantics optimization.

The pass-by-value-then-move pattern is correctly implemented. This allows the compiler to elide copies when rvalues are passed while still supporting lvalues.

Based on learnings

---

118-124: LGTM! Consistent move optimization for key-value insertion.

Both the key and value are correctly moved into storage, following the same optimization pattern as push_back.

Based on learnings

---

126-135: LGTM! Efficient key-value upsert with move semantics.

The implementation correctly moves the value when replacing an existing key (line 132) and moves both key and value when adding a new entry (line 134).

Based on learnings

---

137-144: LGTM! Bulk key-value merge with efficient move semantics.

The implementation correctly moves individual keys and values from the source object. Taking obj by value allows the caller to pass either an lvalue (which will be copied then consumed) or an rvalue (which will be moved then consumed).

Based on learnings

src/univalue/include/univalue.h (1)

80-87: LGTM! Public API declarations correctly updated for move semantics.

The method signatures are correctly updated to match the implementations in univalue.cpp. This API change enables the pass-by-value-then-move optimization pattern across all container modification methods.

Based on learnings

src/wallet/receive.cpp (1)

431-431: LGTM: Efficient iteration with const reference.

The loop now binds _grouping by const reference instead of by value, avoiding unnecessary copies of each std::set<CTxDestination> during iteration. This optimization is consistent with the broader pattern in this PR.

ci/dash/lint-tidy.sh (1)

47-47: LGTM: CI tooling update.

Adding src/util/string.cpp to the iwyu analysis set aligns with the code changes in that file within this PR.

src/util/string.cpp (1)

13-13: LGTM: Correctly removed std::move from const reference.

The change removes std::move(search) where search is a const std::string& parameter. Since you cannot meaningfully move from a const reference, this is the correct approach. The std::regex constructor will copy from the const reference as intended.

src/bench/load_external.cpp (1)

31-31: LGTM: Safe const-reference binding to temporary.

The change binds params as a const reference to the temporary returned by Params(). In C++, this extends the temporary's lifetime to match the reference's scope, and params is used immediately on line 32 within the same scope. This optimization avoids an unnecessary copy without introducing lifetime issues.

src/logging.cpp (1)

465-465: LGTM: Safe const-reference binding avoids copy.

The change binds threadname as a const reference to the result of util::ThreadGetInternalName(), avoiding an unnecessary string copy. The reference is used immediately on line 467 within the same scope, and lifetime extension ensures safety.

src/rpc/util.cpp (1)

102-102: LGTM: Const-reference bindings avoid unnecessary copies.

All three changes bind local variables as const references to UniValue method results (get_str() and getValStr()), avoiding string copies. These methods typically return references to internal storage, and all usages occur within the same function scope:

• Line 102: strHex used on lines 103-107
• Line 129: strNum used on lines 131-132
• Line 138: strNum used on lines 140-141

Also applies to: 129-129, 138-138

src/rpc/rawtransaction_util.cpp (1)

149-149: LGTM: Const-reference bindings to UniValue internal data.

Both changes bind to references of UniValue's internal data structures:

• Line 149: prevTxs references the array and is iterated over starting at line 150
• Line 156: prevOut references the object and is used throughout lines 158-227

These avoid unnecessary copies while remaining safe since the references point to internal storage of the parent UniValue objects that remain alive throughout the usage scope.

Also applies to: 156-156

src/bitcoin-cli.cpp (1)

951-951: No changes required; const-reference binding is safe.

UniValue::get_str() returns const std::string&, which is a reference to internal storage rather than a temporary. The binding in the code is therefore safe—the reference directly aliases the internal string, and no temporary lifetime extension applies. The code is correct as written.

src/evo/mnauth.cpp (1)

39-39: Based on my verification of the codebase, I can now address the original review comment. The first shell script confirmed that mn_activeman.GetPubKey() at line 39 returns CBLSPublicKey (from src/masternode/node.h:80 and src/masternode/node.cpp:292: CBLSPublicKey CActiveMasternodeManager::GetPubKey() const).

The original review comment raises a concern about asymmetry between how line 39 constructs the signHash directly with GetPubKey() versus how ProcessMessage allegedly wraps the public key. However, I need to verify whether the ProcessMessage code at lines 92-94 actually exists as described and whether this inconsistency is genuinely problematic.

Given that:

1. Both code paths use CBLSPublicKey as the public key type
2. The PR summary indicates tests passed
3. The comment references "See comment in PushMNAUTH" suggesting intentional symmetry
4. I cannot locate the exact lines 92-94 in the search results to confirm the alleged asymmetry

The original review comment appears to be speculating about a potential issue without providing definitive evidence of the actual code implementation. The concern is that different methods of creating CBLSPublicKey objects might serialize differently, but this requires:

• Verifying ProcessMessage's actual implementation at lines 92-94
• Confirming whether CBLSPublicKey(dmn->pdmnState->pubKeyOperator.Get()) and GetPubKey() actually produce different serializations

Without access to the complete ProcessMessage implementation in the search results, and given that tests reportedly passed, the review comment's severity cannot be definitively confirmed.

Verify type consistency between PushMNAUTH and ProcessMessage signHash construction.

Line 39 now uses mn_activeman.GetPubKey() directly in the signHash tuple, while the review claims line 92 in ProcessMessage wraps the public key with CBLSPublicKey constructor. Both should serialize identically for MNAUTH to work. Confirm that both code paths produce CBLSPublicKey objects that serialize the same way, especially if they call constructors differently. If tests pass, the serialization is likely compatible, but manually verify the ProcessMessage implementation at lines 92-94 to ensure symmetric signHash computation.

Reviewing files that changed from the base of the PR and between 3c4ff33 and 105af62.

• ci/dash/lint-tidy.sh (1 hunks)
• src/.clang-tidy (1 hunks)
• src/bench/load_external.cpp (1 hunks)
• src/bench/wallet_loading.cpp (0 hunks)
• src/bitcoin-cli.cpp (1 hunks)
• src/bitcoin-tx.cpp (1 hunks)
• src/blockfilter.cpp (1 hunks)
• src/bls/bls_worker.cpp (1 hunks)
• src/coins.cpp (1 hunks)
• src/core_read.cpp (1 hunks)
• src/evo/creditpool.cpp (1 hunks)
• src/evo/mnauth.cpp (1 hunks)
• src/evo/specialtxman.cpp (1 hunks)
• src/init.cpp (1 hunks)
• src/instantsend/instantsend.cpp (1 hunks)
• src/llmq/blockprocessor.cpp (1 hunks)
• src/llmq/snapshot.cpp (1 hunks)
• src/logging.cpp (1 hunks)
• src/masternode/meta.cpp (1 hunks)
• src/netbase.cpp (3 hunks)
• src/node/blockstorage.cpp (1 hunks)
• src/qt/governancelist.cpp (1 hunks)
• src/qt/splashscreen.cpp (2 hunks)
• src/qt/transactionrecord.cpp (0 hunks)
• src/rpc/blockchain.cpp (1 hunks)
• src/rpc/coinjoin.cpp (1 hunks)
• src/rpc/evo.cpp (0 hunks)
• src/rpc/masternode.cpp (0 hunks)
• src/rpc/mining.cpp (3 hunks)
• src/rpc/output_script.cpp (0 hunks)
• src/rpc/rawtransaction_util.cpp (1 hunks)
• src/rpc/util.cpp (3 hunks)
• src/script/descriptor.cpp (2 hunks)
• src/test/base58_tests.cpp (2 hunks)
• src/test/blockfilter_tests.cpp (1 hunks)
• src/test/coinstatsindex_tests.cpp (0 hunks)
• src/test/evo_deterministicmns_tests.cpp (1 hunks)
• src/test/evo_trivialvalidation.cpp (1 hunks)
• src/test/fuzz/txorphan.cpp (1 hunks)
• src/test/key_io_tests.cpp (3 hunks)
• src/test/miniscript_tests.cpp (1 hunks)
• src/test/script_tests.cpp (1 hunks)
• src/test/sighash_tests.cpp (1 hunks)
• src/test/transaction_tests.cpp (4 hunks)
• src/test/util_tests.cpp (1 hunks)
• src/test/validation_block_tests.cpp (1 hunks)
• src/univalue/include/univalue.h (1 hunks)
• src/univalue/lib/univalue.cpp (2 hunks)
• src/util/message.cpp (0 hunks)
• src/util/strencodings.cpp (0 hunks)
• src/util/string.cpp (1 hunks)
• src/util/string.h (0 hunks)
• src/util/threadinterrupt.h (1 hunks)
• src/wallet/bdb.cpp (1 hunks)
• src/wallet/receive.cpp (1 hunks)
• src/wallet/rpc/backup.cpp (8 hunks)
• src/wallet/scriptpubkeyman.cpp (1 hunks)
• src/wallet/spend.cpp (1 hunks)
• src/wallet/test/bip39_tests.cpp (1 hunks)
• src/wallet/test/wallet_tests.cpp (3 hunks)
• src/wallet/wallet.cpp (2 hunks)
• src/wallet/walletdb.cpp (2 hunks)
• test/functional/rpc_wipewallettxes.py (1 hunks)
• test/functional/wallet_balance.py (1 hunks)
• test/functional/wallet_import_rescan.py (5 hunks)
• test/functional/wallet_importdescriptors.py (2 hunks)
• test/lint/run-lint-format-strings.py (0 hunks)

• src/qt/transactionrecord.cpp
• src/util/string.h
• src/test/coinstatsindex_tests.cpp
• src/rpc/evo.cpp
• src/util/strencodings.cpp
• src/rpc/output_script.cpp
• src/rpc/masternode.cpp
• src/util/message.cpp
• src/bench/wallet_loading.cpp
• test/lint/run-lint-format-strings.py

• src/test/fuzz/txorphan.cpp
• src/test/miniscript_tests.cpp

• src/rpc/coinjoin.cpp
• src/qt/governancelist.cpp
• src/util/threadinterrupt.h
• src/evo/specialtxman.cpp
• src/rpc/rawtransaction_util.cpp
• src/core_read.cpp
• src/script/descriptor.cpp
• src/wallet/test/wallet_tests.cpp
• src/test/blockfilter_tests.cpp
• src/node/blockstorage.cpp
• src/instantsend/instantsend.cpp
• src/evo/creditpool.cpp
• src/coins.cpp
• src/blockfilter.cpp
• ci/dash/lint-tidy.sh
• src/test/util_tests.cpp
• src/masternode/meta.cpp
• src/llmq/blockprocessor.cpp
• src/test/key_io_tests.cpp
• src/wallet/receive.cpp
• src/test/base58_tests.cpp
• src/test/evo_trivialvalidation.cpp
• src/test/sighash_tests.cpp
• src/wallet/bdb.cpp
• src/rpc/blockchain.cpp
• src/init.cpp
• src/wallet/scriptpubkeyman.cpp
• src/logging.cpp
• src/util/string.cpp
• src/bitcoin-cli.cpp
• src/test/script_tests.cpp

src/bitcoin-tx.cpp (1)

558-558: LGTM! Valid optimization to avoid unnecessary copy.

Changing from a copy to a const reference is a good optimization that avoids copying the UniValue object on each loop iteration. The reference is used read-only throughout the loop body, making this change safe and beneficial for performance.

src/bls/bls_worker.cpp (1)

18-18: LGTM! Efficient iteration with const reference.

The change from by-value to by-const-reference iteration avoids unnecessary copying while maintaining the same semantics. Since item is only used for read-only operations (IsValid() and GetHash()), the const reference is both correct and more efficient.

src/test/evo_deterministicmns_tests.cpp (1)

889-892: LGTM: Tests assignment operator cache-sharing behavior.

The refactoring from copy-construction to default-construction followed by assignment intentionally exercises a different code path (copy assignment operator) to verify that the SML cache is properly shared. This complements the copy-constructor test at line 886-887.

src/wallet/test/bip39_tests.cpp (1)

28-28: LGTM! Const reference binding reduces copies.

The change from value binding to const reference binding eliminates unnecessary copying of the UniValue test object while ensuring immutability in the test loop.

src/test/transaction_tests.cpp (4)

156-156: LGTM! Const reference binding improves efficiency.

Binding test by const reference instead of copying reduces overhead in the test loop.

---

175-175: LGTM! Const reference binding prevents unnecessary copy.

Binding vinput by const reference eliminates copying of the array returned by get_array().

---

240-240: LGTM! Const reference binding reduces copies.

Binding test by const reference instead of copying improves efficiency in the tx_invalid test loop.

---

259-259: LGTM! Const reference binding eliminates copy overhead.

Binding vinput by const reference prevents copying of the array object in the tx_invalid test path.

src/test/validation_block_tests.cpp (1)

185-185: LGTM! Const reference binding avoids shared_ptr copies.

The change from auto block to const auto& block eliminates unnecessary copying of shared_ptr objects (and associated reference count manipulation) while iterating over the blocks vector.

src/wallet/rpc/backup.cpp (6)

94-95: LGTM! Documentation accurately reflects mempool scanning.

The help text updates correctly clarify that the rescan parameter affects both blockchain and mempool scanning, and provide appropriate guidance for users about when to use rescanwallet.

Also applies to: 100-100

---

213-214: LGTM! Consistent documentation improvements.

The help text updates match the pattern from importprivkey and accurately describe the rescan behavior including mempool scanning.

Also applies to: 223-223

---

411-412: LGTM! Documentation updates are consistent.

The help text correctly describes the rescan parameter behavior including mempool scanning, consistent with the other import RPC methods.

Also applies to: 417-417

---

493-493: LGTM! Clear documentation of rescan behavior.

The note accurately reflects that both blockchain and mempool are rescanned after a successful import.

---

1484-1485: LGTM! Documentation improvements are consistent.

The help text updates accurately describe that rescan operations include both blockchain and mempool scanning, consistent with the other import RPC methods.

Also applies to: 1529-1529

---

1841-1841: LGTM! Documentation complete and accurate.

The timestamp description correctly indicates that mempool scanning is included along with blockchain scanning, completing the consistent documentation improvements across all import RPC methods.

src/qt/splashscreen.cpp (2)

58-58: LGTM: Safe optimization to const reference.

The const reference binding avoids copying the QString while remaining safe—the temporary's lifetime extends through the constructor scope, covering all uses of titleAddText (lines 112, 116, 122).

---

119-119: LGTM: Eliminates unnecessary intermediate variable.

Directly passing the return value of getBadgeColor() to fillRect is a straightforward optimization that avoids an intermediate temporary.

src/.clang-tidy (3)

1-14: LGTM! Expanded clang-tidy checks improve code quality.

The addition of bugprone-use-after-move, misc-unused-using-decls, and the three performance-related checks strengthens static analysis coverage.

---

26-28: LGTM! CheckOptions configuration is appropriate.

Disabling CheckTriviallyCopyableMove for the performance-move-const-arg check reduces false positives when moving trivially copyable types.

---

15-25: Verify that performance-for-range-copy exclusion from WarningsAsErrors matches upstream Bitcoin Core.

This is a backport PR, and backported changes should match the upstream configuration. The performance-for-range-copy check is enabled in the Checks section but not in WarningsAsErrors, while other performance checks are elevated. Confirm this configuration matches the current upstream Bitcoin Core .clang-tidy file to ensure the backport is accurate.

src/llmq/snapshot.cpp (1)

215-216: LGTM! Clean refactoring to eliminate intermediate variable.

The change correctly refactors the code to write directly to response.mnListDiffAtHMinus4C instead of using an intermediate local variable. The defensive reset on failure (line 216) ensures a clean state if BuildSimplifiedMNListDiff partially modified the output before failing.

src/wallet/spend.cpp (1)

316-322: LGTM! Move optimization is safe here.

The change from const COutput& to COutput& enables moving coin objects into the result map. This is safe because AvailableCoinsListUnspent(wallet).all() returns a temporary vector, so moving from its elements avoids unnecessary copies without affecting other code.

src/bench/load_external.cpp (1)

31-31: LGTM! Proper const-reference binding.

The change binds the temporary returned by Params() to a const reference, extending its lifetime to the end of the scope and avoiding an unnecessary copy.

src/netbase.cpp (3)

393-396: LGTM! Simplified error checking.

The direct comparison eliminates the unnecessary intermediate variable assignment when the result is only used once.

---

416-418: LGTM! Consistent with earlier simplification.

---

482-484: LGTM! Consistent pattern applied.

src/rpc/mining.cpp (3)

134-134: LGTM! Const-reference binding avoids copy.

The const reference binding extends the lifetime of the Params() return value through the function scope, avoiding an unnecessary copy of the CChainParams object.

---

359-359: LGTM! Consistent optimization pattern.

---

758-758: LGTM! Avoids string copy.

Binding the result of lpval.get_str() to a const reference avoids copying the string.

src/rpc/util.cpp (3)

102-107: LGTM! Const-reference binding reduces string copies.

The const reference binding to v.get_str() avoids copying the string. The lifetime is safe because v (the UniValue parameter) outlives the strHex reference within the function scope.

---

129-133: LGTM! Consistent optimization pattern.

---

138-142: LGTM! Consistent optimization pattern.

src/univalue/include/univalue.h (1)

80-87: LGTM! Public API change enables move semantics.

These signature changes from const-reference parameters to by-value parameters are intentional to enable move semantics. The implementation (visible in src/univalue/lib/univalue.cpp) uses std::move to efficiently transfer ownership. This is part of the upstream backport and aligns with the broader pattern of move optimizations in this PR.

Based on learnings

src/wallet/wallet.cpp (1)

1685-1690: Mempool scan after full rescan is well‑gated and consistent

The updated comment and the if (!max_height) { ... requestMempoolTransactions(*this); } block correctly extend full rescans to also cover the current mempool, and the cs_wallet locking pattern matches existing usage in postInitProcess. No correctness or locking issues spotted.

Also applies to: 1810-1813

src/univalue/lib/univalue.cpp (1)

104-109: Move‑aware UniValue mutators look correct

Switching these methods to take arguments by value and moving into internal storage avoids extra copies while preserving external semantics. pushKV only moves key after findKey is done with it, and pushKVs iterates over a by‑value copy, avoiding aliasing issues. No issues found.

Also applies to: 118-135, 137-144

src/wallet/walletdb.cpp (1)

887-912: Descriptor cache/key + mnemonic loading logic is coherent

The const‑ref loops over descriptor caches and keys are fine, and the new AddKey/AddCryptedKey calls correctly thread optional mnemonic data from mnemonics / crypted_mnemonics, falling back to empty values for legacy entries. Assumptions about GetScriptPubKeyMan(desc_id) being non‑null match the surrounding descriptor loading logic. No issues identified.

test/functional/wallet_importdescriptors.py (1)

430-459: Multisig mempool visibility checks are well-wired

Capturing send_txid and asserting its presence in node0’s mempool and in wmulti_pub.listunspent(0) correctly verifies that the descriptor-based watch-only wallet sees the unconfirmed multisig spend. This matches the intended mempool-rescan behavior and stays aligned with the upstream backport.

Based on learnings

test/functional/wallet_import_rescan.py (3)

71-103: Confirmations and address-level checks are computed consistently

Using confirmation_height to derive confirmations and applying the same value to both the listtransactions entry and the listreceivedbyaddress entry is consistent, and treating a missing height as “mempool (0 conf)” matches the test’s intent. The optional "trusted" key assertion for confirmed txs is also safe in this context.

---

147-163: Whitelist restart in setup_network is appropriate for mempool-focused tests

Restarting all nodes with [email protected] after seeding coinbase keys is a reasonable way to ensure fast, reliable mempool propagation across the IMPORT_NODES matrix without altering the prune/txindex settings established earlier.

---

203-257: Mempool rescan variants are correctly isolated and verified

The new mempool path (reusing non-pruned, non-late_timestamp variants, creating mempool-only txs, asserting mempool size, and then importing on the appropriate nodes based on rescan flag) cleanly tests that a rescan covers mempool entries while non-rescanning imports leave balances and tx counts at zero. State is reinitialized per variant, so earlier on-chain checks remain unaffected.

Reviewing files that changed from the base of the PR and between 105af62 and c1730d4.

• ci/dash/lint-tidy.sh (1 hunks)
• src/.clang-tidy (1 hunks)
• src/bench/load_external.cpp (1 hunks)
• src/bench/wallet_loading.cpp (0 hunks)
• src/bitcoin-cli.cpp (1 hunks)
• src/bitcoin-tx.cpp (1 hunks)
• src/blockfilter.cpp (1 hunks)
• src/bls/bls_worker.cpp (1 hunks)
• src/coins.cpp (1 hunks)
• src/core_read.cpp (1 hunks)
• src/evo/creditpool.cpp (1 hunks)
• src/evo/mnauth.cpp (1 hunks)
• src/evo/specialtxman.cpp (1 hunks)
• src/init.cpp (1 hunks)
• src/instantsend/instantsend.cpp (1 hunks)
• src/llmq/blockprocessor.cpp (1 hunks)
• src/llmq/snapshot.cpp (1 hunks)
• src/logging.cpp (1 hunks)
• src/masternode/meta.cpp (1 hunks)
• src/netbase.cpp (3 hunks)
• src/node/blockstorage.cpp (1 hunks)
• src/qt/governancelist.cpp (1 hunks)
• src/qt/splashscreen.cpp (2 hunks)
• src/qt/transactionrecord.cpp (0 hunks)
• src/rpc/blockchain.cpp (1 hunks)
• src/rpc/coinjoin.cpp (1 hunks)
• src/rpc/evo.cpp (0 hunks)
• src/rpc/masternode.cpp (0 hunks)
• src/rpc/mining.cpp (3 hunks)
• src/rpc/output_script.cpp (0 hunks)
• src/rpc/rawtransaction_util.cpp (1 hunks)
• src/rpc/util.cpp (3 hunks)
• src/script/descriptor.cpp (2 hunks)
• src/test/base58_tests.cpp (2 hunks)
• src/test/blockfilter_tests.cpp (1 hunks)
• src/test/coinstatsindex_tests.cpp (0 hunks)
• src/test/evo_deterministicmns_tests.cpp (1 hunks)
• src/test/evo_trivialvalidation.cpp (1 hunks)
• src/test/fuzz/txorphan.cpp (1 hunks)
• src/test/key_io_tests.cpp (3 hunks)
• src/test/miniscript_tests.cpp (1 hunks)
• src/test/script_tests.cpp (1 hunks)
• src/test/sighash_tests.cpp (1 hunks)
• src/test/transaction_tests.cpp (4 hunks)
• src/test/util_tests.cpp (1 hunks)
• src/test/validation_block_tests.cpp (1 hunks)
• src/univalue/include/univalue.h (1 hunks)
• src/univalue/lib/univalue.cpp (2 hunks)
• src/util/message.cpp (0 hunks)
• src/util/strencodings.cpp (0 hunks)
• src/util/string.cpp (1 hunks)
• src/util/string.h (0 hunks)
• src/util/threadinterrupt.h (1 hunks)
• src/wallet/bdb.cpp (1 hunks)
• src/wallet/receive.cpp (1 hunks)
• src/wallet/rpc/backup.cpp (8 hunks)
• src/wallet/scriptpubkeyman.cpp (1 hunks)
• src/wallet/spend.cpp (1 hunks)
• src/wallet/test/bip39_tests.cpp (1 hunks)
• src/wallet/test/wallet_tests.cpp (3 hunks)
• src/wallet/wallet.cpp (2 hunks)
• src/wallet/walletdb.cpp (2 hunks)
• test/functional/rpc_wipewallettxes.py (1 hunks)
• test/functional/wallet_balance.py (1 hunks)
• test/functional/wallet_import_rescan.py (5 hunks)
• test/functional/wallet_importdescriptors.py (2 hunks)
• test/lint/run-lint-format-strings.py (0 hunks)

• src/rpc/evo.cpp
• test/lint/run-lint-format-strings.py
• src/util/message.cpp
• src/rpc/output_script.cpp
• src/rpc/masternode.cpp
• src/util/string.h
• src/test/coinstatsindex_tests.cpp
• src/bench/wallet_loading.cpp
• src/util/strencodings.cpp
• src/qt/transactionrecord.cpp

• src/util/threadinterrupt.h

• src/wallet/receive.cpp
• src/.clang-tidy
• src/test/util_tests.cpp
• src/test/fuzz/txorphan.cpp
• src/script/descriptor.cpp
• src/wallet/scriptpubkeyman.cpp
• src/wallet/wallet.cpp
• src/wallet/bdb.cpp
• src/evo/specialtxman.cpp
• src/bitcoin-cli.cpp
• src/test/key_io_tests.cpp
• src/blockfilter.cpp
• src/masternode/meta.cpp
• src/rpc/coinjoin.cpp
• src/rpc/mining.cpp
• src/netbase.cpp
• ci/dash/lint-tidy.sh
• src/test/evo_trivialvalidation.cpp
• src/test/evo_deterministicmns_tests.cpp
• src/test/sighash_tests.cpp
• src/wallet/walletdb.cpp
• src/bls/bls_worker.cpp
• src/node/blockstorage.cpp
• src/llmq/snapshot.cpp
• src/test/miniscript_tests.cpp
• src/test/base58_tests.cpp
• test/functional/wallet_balance.py
• src/wallet/test/wallet_tests.cpp
• src/bitcoin-tx.cpp
• src/logging.cpp
• src/llmq/blockprocessor.cpp

• GitHub Check: mac-build / Build source
• GitHub Check: win64-build / Build source
• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: linux64_ubsan-build / Build source
• GitHub Check: linux64_sqlite-build / Build source
• GitHub Check: linux64_fuzz-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: arm-linux-build / Build source
• GitHub Check: Lint / Run linters

src/coins.cpp (1)

359-359: LGTM! Performance optimization by avoiding unnecessary copies.

The change from auto to const auto& prevents copying each callback object during iteration, which is the standard C++ best practice for range-based for loops when elements don't need to be modified.

src/instantsend/instantsend.cpp (1)

104-110: LGTM! Valid move-semantics optimization.

The change from const auto& to auto& correctly enables moving nodeid_islptr_pair into the return container on line 110, avoiding unnecessary copies. The moved-from entries are properly erased afterward (lines 114-116), making this a standard and safe optimization pattern.

src/qt/splashscreen.cpp (2)

58-58: LGTM! Optimization reduces unnecessary QString copy.

The const reference binding eliminates a QString copy while remaining safe due to C++ lifetime extension semantics.

---

119-119: LGTM! Eliminates unnecessary intermediate variable.

Directly passing the return value to fillRect is cleaner and avoids an unnecessary temporary.

src/wallet/rpc/backup.cpp (6)

94-95: LGTM! Documentation updated to reference mempool scanning.

The help text updates correctly document that rescan now covers both chain and mempool, and provide clear guidance on when to use rescan=false and the rescanwallet command.

Also applies to: 100-100

---

213-214: LGTM! Consistent documentation updates.

The help text updates for importaddress match the pattern from importprivkey, ensuring consistent messaging about mempool scanning across related RPC commands.

Also applies to: 223-223

---

411-412: LGTM! Consistent documentation updates.

The help text updates for importpubkey maintain consistency with the other import commands, providing the same guidance about rescan behavior and mempool scanning.

Also applies to: 417-417

---

493-493: LGTM! Documentation clarifies rescan behavior.

The updated help text clearly indicates that both blockchain and mempool will be rescanned after a successful wallet import.

---

1484-1485: LGTM! Consistent documentation updates.

The help text updates for importmulti maintain consistency with the other import commands, documenting both the rescan guidance and mempool scanning behavior.

Also applies to: 1529-1529

---

1841-1841: LGTM! Documentation updated to reference mempool scanning.

The timestamp description now correctly indicates that mempool will be scanned in addition to the blockchain.

src/wallet/spend.cpp (1)

316-321: Move-enabling ListCoins loop looks correct

Using COutput& here to allow emplace_back(std::move(coin)) is safe: the range-for materializes the std::vector<COutput> temporary for the loop duration, you only move from each element after all its data is consumed, and the temporary vector isn’t used afterward. Behavior stays the same while avoiding an extra copy per output.

src/evo/creditpool.cpp (1)

94-94: LGTM: Reduces overhead by avoiding shared_ptr copies.

The change from value-based to const-reference iteration avoids unnecessary reference count operations on CTransactionRef (a shared_ptr), improving performance without altering behavior.

src/rpc/blockchain.cpp (1)

2530-2534: Valid move optimization – verified and approved.

Changing to non-const reference enables moving scripts into the descriptors map via std::move(script), avoiding copies. Since scripts is a local variable that isn't used after this loop, moving from it is safe and improves performance. Verification confirms no usage of scripts after line 2534.

src/qt/governancelist.cpp (1)

46-64: LGTM! Const-reference optimization avoids copies.

The change from by-value to const-reference bindings when extracting UniValue fields is correct. The temporaries returned by find_value() have their lifetimes extended to the enclosing scope, and all subsequent operations (.isStr(), .getInt<>(), .get_real(), etc.) work correctly with const references.

src/core_read.cpp (1)

192-192: LGTM! Const-reference binding is correct.

Binding strHashType as a const reference to the temporary returned by sighash.get_str() is safe. The temporary's lifetime is extended to the end of the scope, covering all uses (map lookup and error message).

src/init.cpp (1)

1443-1443: LGTM! Const-reference binding is correct.

Binding datadir as a const reference to the temporary returned by gArgs.GetDataDirNet() is safe. The temporary's lifetime is extended through the subsequent calls to DirIsWritable() and LockDirectory().

src/util/string.cpp (1)

13-13: LGTM! Removed unnecessary std::move on const reference.

The change correctly removes std::move() on the search parameter, which is a const reference. Moving from a const reference is not possible and has no effect, so this change aligns with best practices.

src/bench/load_external.cpp (1)

31-31: LGTM! Const-reference binding avoids copy.

Binding params as a const reference to the temporary returned by Params() is correct. The temporary's lifetime is extended to the end of the scope, and the subsequent call to MessageStart() works correctly with a const reference.

src/rpc/rawtransaction_util.cpp (1)

149-156: LGTM! Const-reference bindings avoid copies.

Both changes correctly bind to const references:

• Line 149: prevTxs binds to the array returned by get_array(), extending the temporary's lifetime for the loop.
• Line 156: prevOut binds to the object returned by get_obj(), extending the temporary's lifetime for the iteration.

All subsequent operations work correctly with const references.

src/rpc/util.cpp (1)

102-142: LGTM! Const-reference bindings avoid copies.

All three changes correctly bind string temporaries as const references:

• Line 102 in ParseHashV: strHex bound to v.get_str()
• Line 129 in ParseInt32V: strNum bound to v.getValStr()
• Line 138 in ParseInt64V: strNum bound to v.getValStr()

The temporaries' lifetimes are extended to the end of each function scope, covering all uses (length checks, hex validation, parsing, and error messages).

src/wallet/test/bip39_tests.cpp (1)

28-28: LGTM! Const-reference binding avoids copies in loop.

The change from by-value to const-reference binding for the test element is correct. This avoids copying each UniValue test element during iteration while maintaining the same functionality.

src/test/blockfilter_tests.cpp (1)

145-186: Const-ref binding for JSON test element is safe and avoids copies

const UniValue& test = tests[i]; is used read-only throughout the loop and tests outlives the reference, so this is a pure allocation win with no behavioral change.

src/test/transaction_tests.cpp (1)

155-176: Const-ref UniValue bindings in tx_valid/tx_invalid are correct

Both const UniValue& test = tests[idx]; and const UniValue& vinput = input.get_array(); are used purely read-only, with owning tests/inputs objects alive for the loop. This preserves behavior and reduces copies.

Also applies to: 239-260

src/test/validation_block_tests.cpp (1)

175-190: Const-ref iteration over blocks is thread-safe and avoids extra shared_ptr copies

blocks is populated before the worker threads start and is never mutated, so iterating as for (const auto& block : blocks) is safe under concurrency and slightly cheaper than copying each shared_ptr.

src/test/script_tests.cpp (1)

881-901: Use of const UniValue& test in script_json_test is safe

test is accessed read-only and tests lives for the whole function, so binding by const reference is correct and just reduces copying.

test/functional/rpc_wipewallettxes.py (1)

30-37: Updated txcount expectation after rescan reflects mempool-aware rescan

The added rescanblockchain() and assert_equal(..., 104) after wipewallettxes(True) are consistent with the behavior where a rescan (with no max_height) also inspects the mempool and re-discovers the wiped unconfirmed transaction, restoring the txcount from 103 back to 104. The ordering (asserting gettransaction fails before rescan) correctly separates wipe behavior from rescan behavior.

test/functional/wallet_importdescriptors.py (1)

422-432: Multisig descriptor send tracking and new change address expectation look correct

• Capturing send_txid from wmulti_priv.sendtoaddress(...) and later asserting it is present both in node0’s mempool and in wmulti_pub.listunspent(0) validates that:
  • the multisig spend is relayed across nodes, and
  • the watch‑only descriptor wallet correctly tracks the resulting UTXO.
• The updated change_addr literal is just the deterministic descriptor-derived address at the expected index; no behavioral risk.
• Using bare assert(...) here is consistent with the upstream test style for this backport.

Also applies to: 453-460

src/univalue/lib/univalue.cpp (1)

104-144: UniValue move-based push APIs are semantically safe and improve efficiency

The switch to pass‑by‑value plus std::move in push_back, __pushKV, pushKV, and pushKVs follows the standard “take by value, move into storage” pattern:

• Lookups in pushKV use key before it’s moved, so key search is safe.
• Assigning values[idx] = std::move(val); only moves from the parameter copy, never from the existing element.
• pushKVs(UniValue obj) correctly preserves callers that pass lvalues (they get a copy) and enables efficient moves when given rvalues.

Behavior is preserved while allowing cheaper rvalue usage; no aliasing or lifetime issues are introduced.

src/univalue/include/univalue.h (1)

80-88: Header signatures for UniValue push methods correctly reflect the new move-friendly API

The updated declarations for push_back, __pushKV, pushKV, and pushKVs match the move-based implementations in univalue.cpp and remain source-compatible for existing callers, while enabling efficient rvalue usage. No ABI or semantic issues are apparent.

test/functional/wallet_import_rescan.py (6)

78-82: Whitespace-only change around addresses retrieval

The added blank line after the listreceivedbyaddress call is purely cosmetic and has no functional impact.

---

92-102: Confirmations and mempool handling logic looks correct

• confirmations = (1 + current_height - confirmation_height) if confirmation_height else 0 matches the usual confirmations formula and cleanly handles the mempool case (where confirmation_height is omitted and confirmations should be 0).
• Reusing the same confirmations value for both the tx and address-level confirmations keeps these views consistent.
• The if confirmations: assert "trusted" not in tx guard only triggers for confirmed transactions; for the mempool-rescan checks (where confirmation_height is left as None), confirmations is 0 so this assertion is skipped as intended.

This aligns with the upstream Bitcoin Core behavior being backported.

Based on learnings

---

161-163: Second startup with [email protected] is wired correctly

Restarting all nodes with extra_args=[["[email protected]"]] * self.num_nodes correctly appends the whitelist flag to each node’s existing per-node arguments (including prune/txindex/reindex) defined in self.extra_args, without mutating the stored extra_args themselves. This matches the test framework’s start_nodes / TestNode.start semantics and the prior learning about extra_args handling.

Based on learnings

---

165-165: Blank line before main test body

The added blank line before the main run_test logic is just formatting and does not alter behavior.

---

203-209: Refined loop for second set of confirmed sends

Iterating directly over IMPORT_VARIANTS (without enumerate) to create the second batch of sends keeps semantics identical while staying clear and concise. The per-variant sent_amount, sent_txid, and updated confirmation_height are all set before later checks, so the follow-up balance/txcount assertions remain correct.

---

220-257: New mempool-rescan coverage is coherent and consistent with earlier logic

• Filtering mempool_variants to non-late_timestamp and non-pruned variants matches the intent: mempool rescan is only meaningful on unpruned nodes and doesn’t need the late-timestamp edge case.
• Capturing a single timestamp from the current tip and then sending per-variant txs without mining any blocks ensures all these txs stay in the mempool; assert_equal(len(self.nodes[0].getrawmempool()), len(mempool_variants)) and self.sync_mempools() together guarantee all nodes see the same mempool contents before imports.
• The node selection via ImportNode(variant.prune, expect_rescan) reuses the same mapping as in the confirmed-rescan tests, now restricted to prune=False, so rescan/non-rescan behavior is exercised on the intended nodes.
• For expect_rescan variants, setting confirmation_height = 0 and calling variant.check(variant.initial_txid, variant.initial_amount) (relying on the default confirmation_height=None) drives the new mempool-aware confirmations logic, expecting 0 confirmations and the correct balance/tx count.
• For non-rescan variants, expected balance/txs are explicitly zeroed and variant.check() is called without a txid, so the test asserts the absence of any imported mempool activity.

Overall this block cleanly exercises the mempool-rescan behavior added upstream without introducing Dash-specific regressions.

Based on learnings

Reviewing files that changed from the base of the PR and between fdddfc1 and 001493a.

• contrib/devtools/iwyu/bitcoin.core.imp (1 hunks)
• src/bench/prevector.cpp (2 hunks)
• src/bitcoin-tx.cpp (5 hunks)
• src/core_read.cpp (2 hunks)
• src/evo/deterministicmns.cpp (1 hunks)
• src/evo/specialtxman.cpp (2 hunks)
• src/governance/object.cpp (1 hunks)
• src/instantsend/net_instantsend.cpp (1 hunks)
• src/llmq/signing_shares.cpp (1 hunks)
• src/masternode/meta.cpp (1 hunks)
• src/qt/bitcoinunits.cpp (1 hunks)
• src/test/checkqueue_tests.cpp (1 hunks)
• src/test/coins_tests.cpp (1 hunks)
• src/test/cuckoocache_tests.cpp (1 hunks)
• src/test/fuzz/txorphan.cpp (1 hunks)
• src/test/fuzz/util.h (2 hunks)
• src/test/result_tests.cpp (1 hunks)
• src/test/rpc_tests.cpp (1 hunks)
• src/test/script_tests.cpp (3 hunks)
• src/test/validation_block_tests.cpp (1 hunks)
• src/torcontrol.cpp (2 hunks)
• src/validation.cpp (2 hunks)
• src/wallet/scriptpubkeyman.cpp (2 hunks)

• contrib/devtools/iwyu/bitcoin.core.imp

• src/bitcoin-tx.cpp
• src/test/fuzz/txorphan.cpp
• src/test/validation_block_tests.cpp
• src/test/script_tests.cpp